sock_async: fix cyclic include problem by miri64 · Pull Request #12921 · RIOT-OS/RIOT

miri64 · 2019-12-11T09:28:44Z

Contribution description

Typically a stack needs to add the callback for a sock as a member of its respective sock type so sock_types.h needs to include net/sock/async.h at the moment. As those however include
net/sock/<prot>.h, which in turn include sock_types.h, we create a cyclic dependency.

This fix resolves this cyclic dependency, by putting the callback definitions in its own header that then in turn can be also included by sock_types.h.

Testing procedure

tests/gnrc_sock_async should still work and run

make -C tests/gnrc_sock_async flash test

Issues/PRs references

Required for #12601, #12602, and #12907 to work.

miri64 · 2019-12-11T09:30:25Z

@kaspar030 can you have a look please. This somewhat breaks the "one header for everything" idea we had in #11723, but I don't really see another way to resolve the cyclic dependencies.

miri64 · 2019-12-17T17:16:50Z

LLVM doesn't like re-definitions of types :-(. Any idea how to solve this, or should we just deactivate that warning?

fjmolinas · 2020-01-03T09:28:32Z

LLVM doesn't like re-definitions of types :-(. Any idea how to solve this, or should we just deactivate that warning?

I do not get the warning when compiling RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TOOLCHAIN=llvm make -C tests/gnrc_sock_async clean all, but a solution could be something like:

#if defined (__clang__)
  #pragma clang diagnostic push
  #pragma clang diagnostic ignored "-Wno-typedef-redefinition"
#endif
....
....
#if defined (__clang__)
  #pragma clang diagnostic pop
#endif

for the sections with the redefinitions?

miri64 · 2020-01-03T12:26:26Z

I do not get the warning when compiling RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TOOLCHAIN=llvm make -C tests/gnrc_sock_async clean all, but a solution could be something like:

If you tried the most current version of this PR, this does not surprise me... see my changes to makefiles/toolchain/llvm.inc.mk. The pragma definition you proposed might be a less global approach. Will try soon.

miri64 · 2020-01-03T12:26:54Z

@fjmolinas you have to revert the last commit to get the effect I was describing.

fjmolinas · 2020-01-03T12:42:18Z

@fjmolinas you have to revert the last commit to get the effect I was describing.

I had removed CFLAGS += -Wno-typedef-redefinition, i.e. c4e0bcf, and was not getting warnings.

fjmolinas · 2020-01-03T13:14:34Z

@kaspar030 can you have a look please. This somewhat breaks the "one header for everything" idea we had in #11723, but I don't really see another way to resolve the cyclic dependencies.

@miri64 I tried to look into the discussion in #11723 and to me it seems the idea was to keep the changes restricted and not mix into existing headers. To me adding the async/type.h doesn't violate this. If there are could you summarize the other arguments for the single header approach?

miri64 · 2020-01-06T13:49:08Z

@miri64 I tried to look into the discussion in #11723 and to me it seems the idea was to keep the changes restricted and not mix into existing headers. To me adding the async/type.h doesn't violate this. If there are could you summarize the other arguments for the single header approach?

I think @kaspar030 and I discussed this mostly offline, so its not surprising to not have it there. Basically we wanted to isolate sock_async as much as possible as it is still experimental (in an earlier version the new types and functions were in the respective net/sock/*.h header). That's why we put it all in one header. But you are right, having a separate types header does not contradict this.

miri64 · 2020-01-06T14:33:07Z

I do not get the warning when compiling RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TOOLCHAIN=llvm make -C tests/gnrc_sock_async clean all, but a solution could be something like:

If you tried the most current version of this PR, this does not surprise me... see my changes to makefiles/toolchain/llvm.inc.mk. The pragma definition you proposed might be a less global approach. Will try soon.

BTW you have to build for a board (e.g. for iotlab-m3).

miri64 · 2020-01-06T14:33:29Z

Used pragmas now instead of globally deactivating the redefinition warning.

fjmolinas · 2020-01-06T16:19:53Z

BTW you have to build for a board (e.g. for iotlab-m3).

AHA! that was it, thanks! Test is still passing.

RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 TOOLCHAIN=llvm BOARD=iotlab-m3 make -C

tests/gnrc_sock_async test --no-print-directory
r
/home/francisco/workspace/RIOT2/dist/tools/pyterm/pyterm -p "/dev/riot/tty-iotlab-m3" -b "500000" --noprefix --no-repeat-command-on-empty-line
Connect to serial port /dev/riot/tty-iotlab-m3
Welcome to pyterm!
Type '/exit' to exit.
�Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: buildtest)
UDP event triggered: 0020
UDP message successfully sent
PKTDUMP: data to send:
~~ SNIP  0 - size:  40 byte, type: NETTYPE_UNDEF (0)
00000000  60  00  00  00  90  00  11  00  00  00  00  00  00  00  00  00
00000010  00  00  00  00  00  00  00  00  FE  80  00  00  00  00  00  00
00000020  00  00  00  00  00  00  00  02
~~ SNIP  1 - size:   8 byte, type: NETTYPE_TEST (2)
00000000  01  23  45  67  89  AB  CD  EF
~~ PKT    -  2 snips, total size:  48 byte
IP event triggered: 0020
IP message successfully sent
UDP event triggered: 0010
Received UDP packet from [fe80::2]:38663:
00000000  01  23  45  67  89  AB  CD  EF
IP event triggered: 0010
Received IP packet from [fe80::2]:
00000000  01  23  45  67  89  AB  CD  EF

fjmolinas

Test is still working, usage has been explained. Ignoring typdef redefinition has been limited to the async files and the sock files. I don't think there is another way around this issue.

AFAIKT the single file approach to contain async code is still respected.

@kaspar030 might want to take another look, but otherwise ACK on my side. feel free to squash.

miri64 · 2020-01-07T08:22:30Z

May I squash?

fjmolinas · 2020-01-07T08:47:02Z

@kaspar030 might want to take another look, but otherwise ACK on my side. feel free to squash.

May I squash?

Yes.

Typically a stack needs to add the callback for a sock as a member of its respective `sock` type so `sock_types.h` needs to include `net/sock/async.h` at the moment. As those however include `net/sock/<prot>.h`, which in turn include `sock_types.h`, we create a cyclic dependency. This fix resolves this cyclic dependency, by putting the callback definitions in its own header that then in turn can be also included by `sock_types.h`.

net/sock/async/types.h included by net/sock.h needs to re-typedef the the sock types to prevent cyclic includes.

miri64 · 2020-01-07T09:13:48Z

Done.

fjmolinas · 2020-01-07T12:40:09Z

GO!

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 11, 2019

miri64 added this to the Release 2020.01 milestone Dec 11, 2019

miri64 requested review from benpicco and kaspar030 December 11, 2019 09:28

fjmolinas approved these changes Jan 6, 2020

View reviewed changes

miri64 and others added 4 commits January 7, 2020 10:13

gnrc_sock: use …/async/types.h instead of …/async.h

37d99c1

test/gnrc_sock_async: include net/sock/async.h

82180a8

sock: ignore -Wtypedef-redefinition for LLVM where needed

3896145

net/sock/async/types.h included by net/sock.h needs to re-typedef the the sock types to prevent cyclic includes.

miri64 force-pushed the sock_async/fix/header-foobar branch from 180e613 to 3896145 Compare January 7, 2020 09:13

fjmolinas merged commit d94cd6d into RIOT-OS:master Jan 7, 2020

miri64 deleted the sock_async/fix/header-foobar branch January 9, 2020 11:21

Conversation

miri64 commented Dec 11, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Dec 11, 2019

Uh oh!

miri64 commented Dec 17, 2019

Uh oh!

fjmolinas commented Jan 3, 2020

Uh oh!

miri64 commented Jan 3, 2020

Uh oh!

miri64 commented Jan 3, 2020

Uh oh!

fjmolinas commented Jan 3, 2020

Uh oh!

fjmolinas commented Jan 3, 2020

Uh oh!

miri64 commented Jan 6, 2020

Uh oh!

miri64 commented Jan 6, 2020

Uh oh!

miri64 commented Jan 6, 2020

Uh oh!

fjmolinas commented Jan 6, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 7, 2020

Uh oh!

fjmolinas commented Jan 7, 2020

Uh oh!

miri64 commented Jan 7, 2020

Uh oh!

fjmolinas commented Jan 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants